Skip to content

Add Cluster/Allocation/Explain Query Param Example #4849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add Cluster/Allocation/Explain Query Param Example #4849

wants to merge 5 commits into from
+247 −6

Conversation

joshua-adams-1
Copy link

@joshua-adams-1 joshua-adams-1 commented Jul 9, 2025 •
edited
Loading

Change Details

Extends the documentation for the cluster/allocation/explain API after query parameter support was added in elastic/elasticsearch#129342

  • The query parameters were defined
  • A second request example was provided, highlighting how to pass all parameters in the URL string rather than the request body

Release

To be merged after elastic/elasticsearch#129342

Github Issue - elastic/elasticsearch#127028
Jira Ticket - ES-11865

@joshua-adams-1
Add Cluster/Allocation/Explain Query Param Example
6eab9dd 
Extends the documentation for the `cluster/allocation/explain` API after query parameter support was added in #129342. A second example request was provided with all four query parameters and an empty request body.
@joshua-adams-1
make contrib
695ac41 
Autogenerated output from running `make contrib`
@joshua-adams-1
Merge branch 'main' into explain-cluster-allocation
98bba07
@joshua-adams-1 joshua-adams-1 requested a review from a team as a code owner July 9, 2025 09:47
@joshua-adams-1
Declare query parameters
c1a4840 
Declared the query parameters, ran make contrib and resolved the validation errors
@joshua-adams-1 joshua-adams-1 self-assigned this Jul 9, 2025
@joshua-adams-1 joshua-adams-1 marked this pull request as draft July 9, 2025 10:05
@joshua-adams-1 joshua-adams-1 added documentation skip-backport This pull request should not be backported and removed documentation labels Jul 9, 2025
@joshua-adams-1 joshua-adams-1 marked this pull request as ready for review July 9, 2025 10:08
@joshua-adams-1 joshua-adams-1 mentioned this pull request Jul 9, 2025
Open
lcawl
lcawl reviewed Jul 14, 2025
View reviewed changes
specification/cluster/allocation_explain/ClusterAllocationExplainRequest.ts
@@ -44,6 +45,22 @@ export interface Request extends RequestBase {
}
]
query_parameters: {
/**
* Specifies the node ID or the name of the node to only explain a shard that is currently located on the specified node.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what is meant?

Suggested change
* Specifies the node ID or the name of the node to only explain a shard that is currently located on the specified node.
* Explain a shard only if it is currently located on the specified node name or node ID.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the description verbatim from the existing API docs, see the definition for the current_node parameter. I don't like the existing wording myself but figured it had already passed through a review to have been published. Happy to change if you agree

specification/cluster/allocation_explain/ClusterAllocationExplainRequest.ts
*/
current_node?: string
/**
* Specifies the name of the index that you would like an explanation for.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Specifies the name of the index that you would like an explanation for.
* The name of the index that you would like an explanation for.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, happy to change but will change below as well.

Side question, in this case the same parameters are defined as both query and body parameters. Should they be defined twice, or is there a way to define once and use twice? (So that their descriptions are guaranteed to remain synced?)

specification/cluster/allocation_explain/ClusterAllocationExplainRequest.ts
*/
index?: IndexName
/**
* If true, returns explanation for the primary shard for the given shard ID.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the shard parameter is required when primary is specified? If so, we should mention that in the description.

Suggested change
* If true, returns explanation for the primary shard for the given shard ID.
* If true, returns an explanation for the primary shard for the specified shard ID.

specification/cluster/allocation_explain/ClusterAllocationExplainRequest.ts
*/
primary?: boolean
/**
* Specifies the ID of the shard that you would like an explanation for.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Specifies the ID of the shard that you would like an explanation for.
* An identifier for the shard that you would like an explanation for.

Alternatively, maybe "... that you want explained".

l-trotta
l-trotta reviewed Jul 14, 2025
View reviewed changes
specification/cluster/allocation_explain/ClusterAllocationExplainRequest.ts
/**
* Specifies the node ID or the name of the node to only explain a shard that is currently located on the specified node.
*/
current_node?: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we prefer using aliases rather than primitive types when possible

Suggested change
current_node?: string
current_node?: NodeId

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to change! I had copied the parameter definitions from later in the file, so will change there as well. Can I ask what the benefit of this is? And does this apply to other primitives, like integer, boolean etc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

benefit is that some clients (like .net) use these aliases to provide shortcuts/helpers or for documentation purposes, it applies to most strings, most "time" related numbers, versions numbers and others (boolean is the only one without an alias I think)

l-trotta
l-trotta reviewed Jul 14, 2025
View reviewed changes
...ion/cluster/allocation_explain/examples/request/ClusterAllocationExplainRequestExample2.yaml
# type: request
value: |-
{}
alternatives:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can delete everything below the value field, as language examples are automatically generated

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not know that! Thank you, that's very clever

@joshua-adams-1 @lcawl @l-trotta
Apply suggestions from code review
c370c5d 
Apply suggested improvements from code review

Co-authored-by: Lisa Cawley <[email protected]>
Co-authored-by: Laura Trotta <[email protected]>
@joshua-adams-1
Copy link
Author

Thank you both for the reviews. The majority of the comments regard the parameter descriptions. I copied these verbatim from later in the file, link here:

body: {
    /**
     * Specifies the node ID or the name of the node to only explain a shard that is currently located on the specified node.
     */
    current_node?: string
    /**
     * Specifies the name of the index that you would like an explanation for.
     */
    index?: IndexName
    /**
     * If true, returns explanation for the primary shard for the given shard ID.
     */
    primary?: boolean
    /**
     * Specifies the ID of the shard that you would like an explanation for.
     */
    shard?: integer
  }

Two questions:

  1. Since I copied the descriptions, I am happy to change them in light of the review, but assume I should also change the original?
  2. Is it correct to have the same parameter definitions multiple times in a file? I couldn't find an API in the same position as us, facilitating parameters to be passed both as query parameters but also in the request body

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcawl lcawl lcawl left review comments

@l-trotta l-trotta l-trotta left review comments

@pxsalehi pxsalehi Awaiting requested review from pxsalehi

@DaveCTurner DaveCTurner Awaiting requested review from DaveCTurner

At least 1 approving review is required to merge this pull request.

Assignees

@joshua-adams-1 joshua-adams-1

Labels
skip-backport This pull request should not be backported specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshua-adams-1 @lcawl @l-trotta